flame/utils/mock_flame_core.py (3)

216-220: Improve exception handling: provide message and chain exceptions.

The bare raise Exception (Line 218) loses context and makes debugging difficult. Consider providing a meaningful message and chaining from the original KeyError.

♻️ Proposed fix

         except KeyError:
             if self.stop_event:
-                raise Exception
+                raise Exception(f"Thread stopped due to failure in node {self.stop_event[0]}") from None
             time.sleep(.01)
             pass

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flame/utils/mock_flame_core.py` around lines 216 - 220, Replace the bare
re-raise in the KeyError handler with an exception that preserves context: catch
KeyError as e, and when self.stop_event is truthy raise a new exception (e.g.,
RuntimeError or Exception) with a clear message describing what failed (mention
the operation or key lookup) and use exception chaining (raise ... from e) so
the original KeyError is preserved; update the except block around the code that
references self.stop_event in mock_flame_core.py accordingly.

---

344-344: Rename unused loop variable v to _v.

The variable v is not used within the loop body. By convention, prefix unused variables with underscore.

♻️ Proposed fix

-        for k, v in self.logger.items():
+        for k, _v in self.logger.items():

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flame/utils/mock_flame_core.py` at line 344, Rename the unused loop variable
v to _v in the for k, v in self.logger.items() loop to follow the
unused-variable convention; locate the loop in the mock_flame_core class where
self.logger.items() is iterated and change the second tuple variable from v to
_v (e.g., for k, _v in self.logger.items()), and adjust any subsequent
references (none expected) or tests/lint rules if they assert variable names.

---

47-55: IterationTracker class is simple but not thread-safe.

The increment() method performs a non-atomic read-modify-write on self.iter. Since multiple threads may call __pop_logs__ which calls increment(), this could lead to lost increments (though the practical impact is minimal for logging purposes).

🔧 Optional: Thread-safe increment

+import threading
+
 class IterationTracker:
     def __init__(self):
         self.iter = 0
+        self._lock = threading.Lock()

     def increment(self):
-        self.iter += 1
+        with self._lock:
+            self.iter += 1

     def get_iterations(self):
-        return self.iter
+        with self._lock:
+            return self.iter

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flame/utils/mock_flame_core.py` around lines 47 - 55, The
IterationTracker.increment() does a non-atomic read-modify-write and must be
made thread-safe because __pop_logs__ may call it from multiple threads; add a
threading.Lock (e.g., self._lock) in IterationTracker.__init__ and acquire it
around increment() (and optionally around get_iterations()) to protect self.iter
from concurrent updates, leaving the class API (increment, get_iterations)
unchanged.

flame/star/star_model_tester.py (1)

79-81: ⚠️ Potential issue | 🟡 Minor

Store a string message instead of an Exception object.

Lines 79-81 store an Exception object in thread_errors. When printed at line 100, this will display as Exception('Another thread already failed, ...') rather than just the message text.

🐛 Proposed fix

                 else:
-                    thread_errors[(kwargs['test_kwargs']['role'],
-                                   kwargs['test_kwargs']['node_id'])] = (Exception("Another thread already failed, "
-                                                                                   "stopping this thread as well."))
+                    thread_errors[(kwargs['test_kwargs']['role'],
+                                   kwargs['test_kwargs']['node_id'])] = "Another thread already failed, stopping this thread as well."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flame/star/star_model_tester.py` around lines 79 - 81, thread_errors
currently stores an Exception object for the key (kwargs['test_kwargs']['role'],
kwargs['test_kwargs']['node_id']) which later gets printed and shows the
Exception wrapper; change the stored value to just the error message string
instead of Exception(...) (e.g., "Another thread already failed, stopping this
thread as well.") so printing shows the plain message; update the assignment in
the block that sets thread_errors and ensure any downstream consumers of
thread_errors (the code that prints the error) expect a string rather than an
Exception object, e.g., by leaving their print/format logic unchanged.

flame/star/star_model_tester.py (1)

76-77: Avoid calling dunder methods directly.

__pop_logs__ is a dunder (double-underscore) method, which by convention indicates internal/private behavior. Calling it directly (mock.__pop_logs__(...)) is unconventional. Consider renaming to a single-underscore method (_pop_logs) or a public method (pop_logs) if it's intended to be called externally.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@flame/star/star_model_tester.py` around lines 76 - 77, The review flags a
direct call to the dunder method __pop_logs__ on MockFlameCoreSDK; change
external usage to call a non-dunder method instead and update the class
accordingly: rename or add a public method (e.g., pop_logs or _pop_logs) on
MockFlameCoreSDK to expose the same behavior, update the call site from
mock.__pop_logs__(failure_message=True) to mock.pop_logs(failure_message=True)
(or mock._pop_logs(...) if you want semi-private), and ensure any internal
implementation still delegates to the original logic so behavior is preserved.